pm: honor NUB_CACHE_DIR for the engine cache, and report the env tier in nub config - #740
pm: honor NUB_CACHE_DIR for the engine cache, and report the env tier in nub config#740colinhacks wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
The two behavior changes hold up under scrutiny, but the new test that pins them is not hermetic — it reads the dev box's real ~/.npmrc, which can falsify three of its own controls.
Reviewed changes — full initial review of the single commit on cachedir-env-654.
resolved_cache_dirreads the host cache knob first —settings_context.rsreturnsconfig_env("CACHE_DIR")(NUB_CACHE_DIRunder nub,AUBE_CACHE_DIRstandalone) ahead of the settings chain, so the override reaches the packument caches and the global virtual store rather than only the resolver primer.config get/config listgained the env tier —read_mergedappendsread_env_entries(), so the command reports the value the install will actually use instead ofundefined.env_sourceprovenance helper —env_alias_hitis factored out ofraw_from_envso the reporter and the resolver walk the same alias order under the same brand gate.is_config_env_aliasallow-list — onlynpm_config_*/NPM_CONFIG_*/pnpm_config_*/PNPM_CONFIG_*and the embedder's own prefix read as configuration; ambient vars (CI, both spellings of the proxy vars) do not.primary_entry_keyhoisted fromlist.rs::primary_list_keyintoconfig/mod.rsso env entries key the same way file entries do.- New
pm_env_matrixintegration test — four behavioral rows through the real binary, each with a negative control, usingcache listas the read-side proof of which directory resolved. - Docs and comments — a new cache-directory section in the install docs, plus corrections to four stale comments that described the removed
set_cache_rootseam and the long-gone.npmrc-only gate.
I verified the two load-bearing claims in the new comments independently: cacheDir declares sources.cli = [] and carries no managedPolicy, so the early return neither inverts a CLI tier nor skips an enforcement pass; and AUBE_CACHE_DIR already sat first in the reversed alias walk, so standalone aube's precedence is genuinely preserved. config set / delete / explain, protected-key redaction, and --all default-marking are all unaffected by the read_merged change.
ℹ️ Documenting NUB_CACHE_DIR on the site is a brand-boundary decision, not just a doc addition
The install docs now publish NUB_CACHE_DIR and list it above the neutral npm_config_cache_dir. wiki/agents.md sanctions the knob's existence, but the brand-boundary section in the same file says "never a brand env var as a documented USER knob" and prefers neutral spellings on user-facing surfaces. The PR resolves that tension in favour of publishing, which is defensible — the wiki line it edits already claimed the knob was publicly documented — but it is the maintainer's call rather than a consequence of the bug fix.
Technical details
# Publishing a `NUB_*` knob as documented user surface
## Affected sites
- `site/content/docs/install/index.mdx:604-607` — `NUB_CACHE_DIR` listed first, `npm_config_cache_dir` second.
- `wiki/agents.md:396` — edited to record that `NUB_CACHE_DIR` is now documented and the other two knobs are not.
- `wiki/agents.md` brand-boundary section — "Internal `NUB_*` env vars are fine; never a brand env var as a documented USER knob."
## Required outcome
- A deliberate answer to: does Nub publish `NUB_CACHE_DIR` as supported user surface, or keep it live-but-undocumented and publish only the neutral `npm_config_cache_dir` spelling?
## Suggested approach (optional)
- If publishing: lead the docs block with the neutral `npm_config_cache_dir` and present `NUB_CACHE_DIR` as the Nub-branded equivalent, matching the stated preference for neutral names.
- If not: drop the `NUB_CACHE_DIR` line from the mdx and leave the `wiki/agents.md` correction, which is accurate either way.
## Open questions for the human
- The `resolved_cache_dir` fix is orthogonal to this and should land regardless.ℹ️ Nitpicks
site/content/docs/install/index.mdx:616— "split them and every install degrades to a per-file copy" holds only while the global virtual store is enabled.enableGlobalVirtualStoreis off under CI, and a CI cache mount is one of the two motivating examples on this page. Thesettings.tomlcacheDirdocs carry the qualifier ("on installs with the global virtual store enabled"); the mdx drops it.vendor/aube/crates/aube/src/commands/settings_context.rs:527-529— the comment correctly notes that acacheDirthat grows amanagedPolicymust not be short-circuited here, but nothing enforces it. Adebug_assert!onmeta.managed_policy.is_empty()would turn the note into a tripwire, if that feels proportionate.
Claude Opus | 𝕏
…ng .npmrc Review round on #740. `pm_env_matrix` pinned the cache and data dirs but not `HOME`, so every row read the developer's real user `.npmrc`. Reproduced: a `cache-dir` line there falsifies three of the file's own controls — `cache list` names the leaked directory instead of nothing, and both the plain and `--global` reads of `cache-dir` report it instead of `undefined`. CI has a clean `HOME`, so this would have gone red on one machine and nowhere else. Pin `HOME`, `USERPROFILE` and `XDG_CONFIG_HOME` to fixture paths, and scrub both `userconfig` spellings, which relocate the user `.npmrc` even once `HOME` is pinned. Add the row for both cache-dir spellings set at once. That combination is where the precedence rule is encoded twice — the early return in `resolved_cache_dir` and the push order of the config env tier — so a drift between them makes `config get` report a cache the install is not using, which is the invariant the change exists to restore. The config TUI rendered the merged read under the label `Effective .npmrc value:`. That read spans every source, and since the env tier joined it the value shown is routinely one no file holds, so the label names the wrong source; it is now `Effective value:`. The edit path is unaffected — it reads project scope. A `debug_assert` turns the note about `cacheDir` carrying no `managedPolicy` into a tripwire, since the enforcement it would skip runs at the end of the generated accessor. Docs: the per-file-copy warning applies only while the shared virtual store is enabled, which is off in CI — one of the two motivating examples on that page. The `nub link -g` registry is a fourth cache that stays at the platform default. Lead the environment block with the neutral `npm_config_cache_dir`, matching the preference for neutral spellings on user-facing surfaces.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes — the delta since the 79656035 review, which is the single follow-up commit fb98c351. It takes every point from the prior review; all four inline threads are resolved.
pm_env_matrix::runis now isolated from host config —HOME,USERPROFILEandXDG_CONFIG_HOMEare pinned to a fixture-ownedhome/, and bothuserconfigspellings joinedSCRUBBED.aube_store::dirs::cache_dir()consultsXDG_CACHE_HOMEahead of the%LOCALAPPDATA%branch on every platform, so the existing cache pin holds on Windows as well.- Added
both_cache_dir_spellings_agree_across_surfaces— setsNUB_CACHE_DIRandnpm_config_cache_dirto two differently-seeded directories and assertscache listandconfig get cache-dirboth name the branded one. This is a real test rather than a tautology:config getresolves throughread_merged, a code path independent ofresolved_cache_dir, so inverting either encoding of the precedence rule turns it red. debug_assert!guarding the early return —meta::find("cacheDir").is_none_or(|m| m.managed_policy.is_empty()).cacheDirdeclares nomanagedPolicyinsettings.toml, andSettingMeta::managed_policyis a&'static str, so the assert holds today and fires the moment the setting grows a policy the early return would skip.- TUI detail pane relabeled
Effective value:— the merged read now spans the env tier, so the old.npmrcwording named the wrong source. - Install docs corrected — the neutral
npm_config_cache_dirnow leads the env pair, the cross-volume warning is scoped to installs with the shared virtual store on (whichsettings.tomlconfirms is off under CI), and the global-links registry joins the list of caches that stay at the platform default.
I re-verified the load-bearing claims in the new copy against the code rather than the prior review: global_links_dir() is cache_dir().join("global-links") off the platform default, so it genuinely does not follow a cacheDir override; and enableGlobalVirtualStore is documented as on outside CI and off under it.
ℹ️ Nitpicks
vendor/aube/crates/aube-settings/settings.toml:2874-2876— thecacheDirsetting's own docs still list three caches that stay at the platform default (OSV mirror,node-gyp, git clones). That is the same list the mdx just gained a fourth entry for, and it is the copynub configand the TUI render, so the two surfaces now disagree.vendor/aube/crates/aube/src/commands/config/tui.rs:454— the localnpmrc_effectivekept its name after the label becameEffective value:, which reads slightly against the new comment directly below it explaining that the value routinely comes from no file.
Claude Opus | 𝕏
…cache list Review round 2 on #740, all three accuracy points. `pm_env_matrix`'s new doc comment claimed every config root was pinned, but the global and builtin `.npmrc` paths derive from `NPM_CONFIG_PREFIX` / `npm_config_prefix` / `PREFIX` (`resolve_global_npmrc_paths`), with `globalconfig` and `builtin_config` as direct overrides in the same family. None were scrubbed, so running the suite from an npm script — or in any shell exporting `PREFIX` — put `<prefix>/etc/npmrc` back in the chain. A `cache-dir` line there is unlikely, so this was the claim being inexact rather than a live falsifier; scrubbing the family makes it exact. The `cacheDir` setting's own docs listed three caches that stay at the platform default while the install docs now list four. That copy is what `config explain` and the TUI render, so the two surfaces disagreed; the global-links registry joins the list. Rename the TUI's `npmrc_effective` to `effective`, which is what the relabelled line and the comment above it now describe.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor consistency nit inline.
Reviewed changes — the delta since the fb98c351 review, which is the single follow-up commit 96567771. It takes all three accuracy points from that round; the one inline thread is resolved.
SCRUBBEDgained the npmrc path-redirect family —npm_config_prefix/NPM_CONFIG_PREFIX/PREFIXplus theglobalconfigandbuiltin_configspellings, with the rewritten doc comment namingresolve_global_npmrc_pathsas the reason. That makesrun()'s "every config root is pinned" claim exact.cacheDir's own docs name the global-links registry —settings.tomlnow lists four caches that stay at the platform default, matching the list the install docs gained last round.- TUI local renamed to
effective— follows theEffective value:relabel and the comment below it explaining the value routinely comes from no file.
I checked the two load-bearing claims against the code rather than the commit message. resolve_global_npmrc_paths (aube-registry/src/config/load.rs:948-973) reads exactly the seven variables now scrubbed and nothing else, so the family is complete rather than merely longer. global_links_dir() is cache_dir().join("global-links") (aube-store/src/dirs.rs:20) with no cacheDir consultation, so it genuinely belongs on the stays-put list.
Claude Opus | 𝕏
… in nub config
An env-set `cacheDir` was inert in 0.6.0: a hand-written `.npmrc`-only
presence gate ran ahead of the settings resolver, so `npm_config_cache_dir`
and `NPM_CONFIG_CACHE_DIR` were accepted and discarded while the `.npmrc`
spelling worked. The v1.35.0 engine sync dropped that gate and v0.7.0 shipped
the fix, but nothing pinned it and two related defects were still live.
`resolved_cache_dir` now reads the host's first-class `config_env("CACHE_DIR")`
knob ahead of the settings chain. Only the resolver primer read it before, so
the one spelling under nub's own brand moved a subdirectory of the cache
rather than the cache. Standalone aube is unchanged: `config_env` composes the
same `AUBE_CACHE_DIR` the settings table declares, which already outranked the
`npm_config_*` forms, so this preserves that order rather than introducing one.
`config get` and `config list` gained the env tier. `read_merged` composed
embedder defaults plus files, so an env-set setting was invisible to both — an
install moved the cache while `config get cache-dir` printed `undefined`. The
tier is appended last, matching `cli > env > files`. Only config-carrying
variables are surfaced: the `npm_config_*` family, its pnpm sibling, and the
active brand's prefix. An ambient variable a setting merely observes is not
configuration and stays out, which is where npm draws the line. That test is
an allow-list because several settings declare both spellings of a proxy var,
so a deny-list naming the uppercase forms would hide one and report the other.
Scoped reads are unchanged: `--local` and `--global` ask about a file.
The new `pm_env_matrix` test pins every spelling, the precedence, and both
exclusions, offline, each row against a control that fails when the behavior
is absent. Four comments describing the removed gate and a since-deleted
`set_cache_root` registration are corrected, and the cache directory is now
documented.
Closes #654
…ng .npmrc Review round on #740. `pm_env_matrix` pinned the cache and data dirs but not `HOME`, so every row read the developer's real user `.npmrc`. Reproduced: a `cache-dir` line there falsifies three of the file's own controls — `cache list` names the leaked directory instead of nothing, and both the plain and `--global` reads of `cache-dir` report it instead of `undefined`. CI has a clean `HOME`, so this would have gone red on one machine and nowhere else. Pin `HOME`, `USERPROFILE` and `XDG_CONFIG_HOME` to fixture paths, and scrub both `userconfig` spellings, which relocate the user `.npmrc` even once `HOME` is pinned. Add the row for both cache-dir spellings set at once. That combination is where the precedence rule is encoded twice — the early return in `resolved_cache_dir` and the push order of the config env tier — so a drift between them makes `config get` report a cache the install is not using, which is the invariant the change exists to restore. The config TUI rendered the merged read under the label `Effective .npmrc value:`. That read spans every source, and since the env tier joined it the value shown is routinely one no file holds, so the label names the wrong source; it is now `Effective value:`. The edit path is unaffected — it reads project scope. A `debug_assert` turns the note about `cacheDir` carrying no `managedPolicy` into a tripwire, since the enforcement it would skip runs at the end of the generated accessor. Docs: the per-file-copy warning applies only while the shared virtual store is enabled, which is off in CI — one of the two motivating examples on that page. The `nub link -g` registry is a fourth cache that stays at the platform default. Lead the environment block with the neutral `npm_config_cache_dir`, matching the preference for neutral spellings on user-facing surfaces.
…cache list Review round 2 on #740, all three accuracy points. `pm_env_matrix`'s new doc comment claimed every config root was pinned, but the global and builtin `.npmrc` paths derive from `NPM_CONFIG_PREFIX` / `npm_config_prefix` / `PREFIX` (`resolve_global_npmrc_paths`), with `globalconfig` and `builtin_config` as direct overrides in the same family. None were scrubbed, so running the suite from an npm script — or in any shell exporting `PREFIX` — put `<prefix>/etc/npmrc` back in the chain. A `cache-dir` line there is unlikely, so this was the claim being inexact rather than a live falsifier; scrubbing the family makes it exact. The `cacheDir` setting's own docs listed three caches that stay at the platform default while the install docs now list four. That copy is what `config explain` and the TUI render, so the two surfaces disagreed; the global-links registry joins the list. Rename the TUI's `npmrc_effective` to `effective`, which is what the relabelled line and the comment above it now describe.
Rebasing onto #644 put "Relocating the store" directly above the new cache directory section, and it already gives the same move-both-together advice from the store side. Stop restating it and cross-reference instead, keeping only what that section does not say: the pairing matters solely while the shared virtual store is on, which is off under CI.
9656777 to
c195627
Compare
Review round 3 on #740. The platform-default cache list exists in three places, and the prior commit updated only two of them — `settings.toml`'s `cacheDir` docs and the install docs page — so this comment deferred to "the setting itself" while enumerating one fewer entry than the setting does. Nothing checks that the three agree, which is now said out loud where the next person adding a consumer will read it.

Closes #654.
Env-set
cacheDirwas inert in 0.6.0: an.npmrc-only gate ran ahead of the settings resolver. #621 dropped it and v0.7.0 shipped the fix, with nothing pinning it. Two related defects were still live.resolved_cache_dirreadsconfig_env("CACHE_DIR")ahead of the settings chain, soNUB_CACHE_DIRmoves the engine cache, not just the resolver primer. Byte-identical for standalone aube.nub config get/config listgained the env tier. They read files only, so an install moved the cache whileconfig get cache-dirprintedundefined. Only config-carrying vars surface; ambient ones (CI,http_proxy) do not, matching npm. Scoped reads unchanged.pm_env_matrixtest pins both. Four stale comments corrected; the cache directory is now documented.